-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add the fields for signing to crd #91
Conversation
Hi @chr15p. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
@qbarrand We need to trigger the Github actions here as well. I don't think I have enough privileges to do it (can't see the |
Can you please add a commit message describing those changes? |
KeySecret *v1.LocalObjectReference `json:"keySecret"` | ||
|
||
// a secret containing the public key used to sign kernel modules for secureboot | ||
CertSecret *v1.LocalObjectReference `json:"certSecret"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a public key for signing purposes?
Few questions: |
written a better commit message that hopefully explains my thinking (I've been staring at this stuff so long I forget it isn't obvious to other people! ) The signing process itself will use the sign-file binary distributed with the kernel source (see https://www.kernel.org/doc/html/v4.15/admin-guide/module-signing.html#manually-signing-modules) and that requires the public and private key, so the simple answer is we need both keys/certs as input parameters to that code. We could write our own signing code but that seems fragile. |
Thanks for updating the commit message.
How DTK is related here? We don't have DTK u/s. I guess you meant "driver container" instead of "dtk"?! (If this is the case, please update the commit message as well). Besides that, LGTM. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chr15p, ybettan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small comments, lgtm otherwise. Thanks!
Codecov ReportBase: 73.46% // Head: 73.46% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #91 +/- ##
=======================================
Coverage 73.46% 73.46%
=======================================
Files 17 17
Lines 1771 1771
=======================================
Hits 1301 1301
Misses 404 404
Partials 66 66 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
this add four fields to the crd in their own subsection: FileList - a list of kmod files within the container to sign KeySecret and CertSecret - these are the names of secrets containing the private and public keys respectivly that will be used to sign the kmods. The public key should be the one added into the uefi moklist for secureboot, and KeySecret is its private component. (See the kernel docs e.g. https://www.kernel.org/doc/html/v4.15/admin-guide/module-signing.html#manually-signing-modules for details of this process) unsignedImage - the optional name of a dtk image containing the unsigned knods, used only if their is no build stage. The expected workflow will be (not implemented in this commit): if there is only a kernelmapping.build section it will produce the image kernelmapping.containerImage (for the non-secureboot case) if there is both km.build and km.sign then build will produce an intermediate image and sign will consume that to produce km.containerImage if there is only km.sign signing will consumse km.sign.unsignedImage and produce km.containerImage (this is for the case where a vendor supplies prebuilt images that need signing) the generation of the intermediate image name will be handled automatically within the controller (again in a different commit) but will probably be something like km.containerImage + "-unsigned"
/lgtm |
Signed-off-by: Michail Resvanis <mresvani@redhat.com> Signed-off-by: Michail Resvanis <mresvani@redhat.com>
…gs#92) this add four fields to the crd in their own subsection: FileList - a list of kmod files within the container to sign KeySecret and CertSecret - these are the names of secrets containing the private and public keys respectivly that will be used to sign the kmods. The public key should be the one added into the uefi moklist for secureboot, and KeySecret is its private component. (See the kernel docs e.g. https://www.kernel.org/doc/html/v4.15/admin-guide/module-signing.html#manually-signing-modules for details of this process) unsignedImage - the optional name of a dtk image containing the unsigned knods, used only if their is no build stage. The expected workflow will be (not implemented in this commit): if there is only a kernelmapping.build section it will produce the image kernelmapping.containerImage (for the non-secureboot case) if there is both km.build and km.sign then build will produce an intermediate image and sign will consume that to produce km.containerImage if there is only km.sign signing will consumse km.sign.unsignedImage and produce km.containerImage (this is for the case where a vendor supplies prebuilt images that need signing) the generation of the intermediate image name will be handled automatically within the controller (again in a different commit) but will probably be something like km.containerImage + "-unsigned" Co-authored-by: Chris Procter <cprocter@redhat.com>
Add fields to the CRD to support kmod signing
This PR just adds the fields, the functionality to actually use them will come in a later PR. This is mostly intended for discussion, to ensure the actual code is built on foundations we agree on, and to try and keep the size of that future PR down to a reviewable and mergeable size (which my previous attempts have not been great at!)
Edit:
updated commit message to say
this add four fields to the crd in their own subsection:
FileList - a list of kmod files within the container to sign
KeySecret and CertSecret - these are the names of secrets containing the private and public keys respectivly that
will be used to sign the kmods. The public key should be the one added into the uefi moklist for secureboot,
and KeySecret is its private component. (See the kernel docs e.g.
https://www.kernel.org/doc/html/v4.15/admin-guide/module-signing.html#manually-signing-modules
for details of this process)
unsignedImage - the optional name of a dtk image containing the unsigned knods, used only if their is no build stage.
The expected workflow will be (not implemented in this commit):
if there is only a kernelmapping.build section it will produce the image kernelmapping.containerImage
(for the non-secureboot case)
if there is both km.build and km.sign then build will produce an intermediate image and sign will
consume that to produce km.containerImage
if there is only km.sign signing will consumse km.sign.unsignedImage and produce km.containerImage
(this is for the case where a vendor supplies prebuilt images that need signing)
the generation of the intermediate image name will be handled automatically within the controller
(again in a different commit) but will probably be something like km.containerImage + "-unsigned"